feat!: Refactor URLs in status and extend protocol support#412
feat!: Refactor URLs in status and extend protocol support#412farshidtz wants to merge 10 commits into
Conversation
Consistent naming conventions Remove obsolete wrapper func endpoint cleanup
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy “endpoints” status output with a richer “listeners” representation so server connectivity can be reported for additional protocols (notably WebSocket and WebSocket-over-Unix-socket), while keeping export-status output backward-compatible by continuing to emit endpoints (URLs only).
Changes:
- Replace
Status.EndpointswithStatus.Listenersand updateSnapStatus+ its unit test accordingly. - Introduce
ServerListenerswith support forhttp/https,ws, andws+unix, plus comprehensive unit tests; remove the oldendpointsimplementation and tests. - Update chat/webui commands to use
OpenAiBaseUrl, and keepexport-statusemitting legacyendpointsandopenai.json.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/modelctl/common/status.go | Switch status payload from endpoints to listeners and fetch listeners via ServerListeners. |
| cmd/modelctl/common/status_test.go | Update assertions to validate listener URL instead of endpoint URL. |
| cmd/modelctl/common/listeners.go | New listeners implementation with extended protocol support + helper accessors. |
| cmd/modelctl/common/listeners_test.go | New tests covering multiple servers, ws/ws+unix, and unsupported protocols. |
| cmd/modelctl/common/endpoints.go | Removed old endpoints implementation (replaced by listeners). |
| cmd/modelctl/common/endpoints_test.go | Removed old endpoints tests (replaced by listeners tests). |
| cmd/modelctl/commands/webui.go | Use OpenAiBaseUrl instead of removed OpenAiEndpoint. |
| cmd/modelctl/commands/serve-webui.go | Use OpenAiBaseUrl instead of removed OpenAiEndpoint. |
| cmd/modelctl/commands/chat.go | Use OpenAiBaseUrl instead of removed OpenAiEndpoint. |
| cmd/modelctl/commands/export-status.go | Convert listeners → legacy endpoints (URLs) for backward compatibility and guard empty openai endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (2)
cmd/modelctl/common/endpoints.go:177
- Same TrimRight issue here: TrimRight uses a character cutset and may trim more than the intended "+unix" suffix. TrimSuffix avoids accidental trimming and better communicates intent.
protocol := strings.TrimRight(server.Protocol, "+unix") // remove +unix suffix for URL scheme
unixSocketUrl := fmt.Sprintf("%s://unix%s", protocol, server.BasePath)
cmd/modelctl/commands/status_test.go:88
- This example calls statusCommand.run(), which starts a progress spinner that writes to stdout and will break Go example output matching. Replace the run() call with statusJson() and print the returned string (requires fmt import).
func Example_statusCommand_printStatusJson() {
ctx := createTestContextForStatus()
cmd := statusCommand{
Context: ctx,
format: "json",
}
if err := cmd.run(nil, nil); err != nil {
log.Fatalf("failed to print status in json format: %v", err)
}
imatrisciano
left a comment
There was a problem hiding this comment.
This looks solid, I've left a couple of comments
| case "http", "https": | ||
| httpUrl, err := serverHttpUrl(ctx, serverSettings) | ||
| entrypoint, err = serverHttpEntrypoint(ctx, serverSettings) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting server HTTP URL: %v", err) | ||
| return nil, fmt.Errorf("constructing HTTP entrypoint: %v", err) | ||
| } | ||
| case "http+unix", "https+unix": | ||
| entrypoint, err = serverHttpOverUnixSocketEntrypoint(ctx, serverSettings) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("constructing HTTP Unix entrypoint: %v", err) | ||
| } | ||
| case "ws": | ||
| entrypoint, err = serverWsEntrypoint(ctx, serverSettings) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("constructing WebSocket entrypoint: %v", err) | ||
| } | ||
| case "ws+unix": | ||
| entrypoint, err = serverWsOverUnixSocketEntrypoint(ctx, serverSettings) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("constructing WebSocket Unix entrypoint: %v", err) |
There was a problem hiding this comment.
If https is supported, arguably wss should be supported as well
| func (server Server) validate(name string) error { | ||
| if server.Protocol == "" { | ||
| return fmt.Errorf("required field is not set for server %s: protocol", name) | ||
| } | ||
|
|
||
| if server.BasePath == "" { | ||
| return fmt.Errorf("required field is not set for server %s: base-path", name) | ||
| // base-path is optional | ||
| if server.BasePath != "" { | ||
| if !strings.HasPrefix(server.BasePath, "/") { | ||
| return fmt.Errorf("invalid base-path for server %s: must start with '/'", name) | ||
| } | ||
|
|
||
| parsed, err := url.Parse(server.BasePath) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid base-path for server %s: %v", name, err) | ||
| } | ||
|
|
||
| if parsed.Scheme != "" || parsed.Host != "" || parsed.Opaque != "" { | ||
| return fmt.Errorf("invalid base-path for server %s: must be a URL path, not a full URL", name) | ||
| } | ||
|
|
||
| if parsed.RawQuery != "" || parsed.Fragment != "" { | ||
| return fmt.Errorf("invalid base-path for server %s: query and fragment are not allowed", name) | ||
| } | ||
| } |
There was a problem hiding this comment.
This validation function should verify that the value of server.Protocol is one of the supported ones, as defined in cmd/modelctl/common/endpoints.go
Since namespaces are used to identify a configuration group, should namespaces containing dots be rejected?
| return fmt.Errorf("getting status: %v", err) | ||
| } | ||
|
|
||
| // Convert Entrypoints to Endpoints (extract URLs) for backward compatibility |
There was a problem hiding this comment.
I would add a link to this PR for future reference
| // Convert Entrypoints to Endpoints (extract URLs) for backward compatibility | |
| // Convert Entrypoints to Endpoints (extract URLs) for backward compatibility, | |
| // see https://github.com/canonical/inference-snaps-cli/pull/412 |
Change status data model from:
To:
And add support for "http+ws", "ws", "ws+unix" protocols in runtime manifests. Example status:
Example status in JSON:
{ "engine": "cpu", "services": { "llama-server": "active" }, "entrypoints": { "logger": { "url": "http://localhost:8081/" }, "openai": { "url": "http://0.0.0.0:8080/v1" }, "whisperlive": { "unix-socket": "/run/whisper.sock", "unix-socket-url": "ws://unix/realtime" } }, "model": { "name": "gemma-4-4b-it-int4-fq-ov" } }Add support for config namespacing in runtime manifests to allow the definition of servers listening on separate ports.
The namespace is used to retrieve the configurations for a server entry to construct the corresponding entrypoint. In the above example, the
openaiandkserveservers URLs will be constructed by using thehttp.portandhttp.hostconfigurations (no namespace). For logger, the configuration will be taken fromlogger.http.portandlogger.http.hostinstead.BREAKING CHANGE: This is a breaking change for the clients that rely on the
endpoints.<key>field in status. They should instead queryentrypoints.<key>.url.